SOLR-18248 : Migration of Tasks V2 APIs to JAX-RS Constructs#4452
SOLR-18248 : Migration of Tasks V2 APIs to JAX-RS Constructs#4452jaykay12 wants to merge 19 commits into
Conversation
|
|
||
| import static org.apache.solr.client.api.util.Constants.INDEX_PATH_PREFIX; | ||
|
|
||
| @Path(INDEX_PATH_PREFIX + "/tasks/listjalaz") |
There was a problem hiding this comment.
;-) Look slike you have some debugging?
There was a problem hiding this comment.
yeah, started with firstly wiring this api to give any random response, & now using this to compare my V2 response with the original V2 response, 😄 thus, kept a different api endpoint for now to do response comparision.
will surely update this during self-review before raising for formal review 💯
epugh
left a comment
There was a problem hiding this comment.
Thanks for getting started on this, I went ahead and put some comments in that may be too early to be added ;-)
| @GET | ||
| @StoreApiParameters | ||
| @Operation( | ||
| summary = "Lists all the currently running tasks or status of any taskUUID being passed as queryParam", |
There was a problem hiding this comment.
So, I think we would revamp this api... if you look at the desired api, ListActiveTasks would just list all tasks. If you wanted a specfific task, then it would be a idfferent end point with a /taskUUID.
There was a problem hiding this comment.
yes, have updated this Api class for now, handling the taskUUID as a path param now, it was queryParam in the V1 as well as homegrown v2.
|
|
||
| public class ListActiveTaskResponse extends SolrJerseyResponse { | ||
| @JsonProperty | ||
| public Map<String, String> taskList; |
There was a problem hiding this comment.
We are trying to move to more strongly typed responses so taht we don't have Map everywhere. Go ahead and specify out the return properties!
There was a problem hiding this comment.
Yes, have done so.
|
|
||
| } | ||
|
|
||
| private void handleStandAloneMode(ListActiveTaskResponse response, String taskUUID) { |
There was a problem hiding this comment.
One thing we are trying to do is move the V1 API logic into the V2 code base, and have the V1 call the V2 version, this way, as these apis evolve, we know that v1 and v2 stay in sync till we get rid of V1. I believe that ideally we would want ActiveTasksListComponent to delegate to the same code as this V2 api. Now, maybe it's so simple that both can just call core getCancellableQueryTracker().getActiveQueriesGenerated. In other V2 apis, you can see the code live in the V2 and then V1 calls it...
There was a problem hiding this comment.
understood this part, have made suitable changes.
| log.debug("solr cloud"); | ||
| } | ||
| handleSolrCloudMode(response, taskUUID); | ||
| } else { |
There was a problem hiding this comment.
not quite sure I see the differences in the two paths yet?
There was a problem hiding this comment.
yea, i am also checking this up.
I have tested this endpoint by running on cloud example which we have in examples, it worked fine. It should work fine for standalone as well, will remove the current conditional branching done once that's verified.
thank you so much @epugh for the prompt reviews & initial guidance, much appreciated. I will keep addressing your comments as we go, I kept this PR as draft for now as per our community guidelines, but yea thanks again for reviewing this at such initial state, so that i can course correct myself promptly. ✨ |
jaykay12
left a comment
There was a problem hiding this comment.
self review ✅
[pending] to add tests.
https://issues.apache.org/jira/browse/SOLR-18248
Description
Migrating 2 out of the 3 V2 APIs of Tasks, from existing homegrown annotations to JAX-RS Standard constructs & annotations
Solution
Have exposed 1 new interface: ListActiveTasksApi which serves the
/lists/taskas well as/list/task/<taskId>endpoints.ListActiveTasks is the main class which takes on the responsibility for v2 path as well as also for the v1 api path via ActiveTasksListHandler
2 new DTO for List Task API
1 new DTO for Task Status API
Removed the now deprecated files: ListActiveTasksAPI & ActiveTasksListComponent from the codebase.
No Coding Agent used.
Tests
Please describe the tests you've developed or run to confirm this patch implements the feature or solves the problem.
Checklist
Please review the following and check all that apply:
mainbranch../gradlew check.